Skip to content

Security/StaticStrreplace: various sniff improvements#847

Merged
GaryJones merged 6 commits intodevelopfrom
feature/security-staticstrreplace-sniff-review
Jul 18, 2025
Merged

Security/StaticStrreplace: various sniff improvements#847
GaryJones merged 6 commits intodevelopfrom
feature/security-staticstrreplace-sniff-review

Conversation

@jrfnl
Copy link
Copy Markdown
Collaborator

@jrfnl jrfnl commented Jul 18, 2025

Security/StaticStrreplace: extend AbstractFunctionParameterSniff

As things were, the determination of whether or not a T_STRING is a call to the global PHP native str_replace() function was severely flawed.

By switching the sniff over to be based on the WordPressCS AbstractFunctionParameterSniff class, this flaw is mitigated.

Includes adding a slew of additional tests, some of which (line 8 - 13) are specific to the flaw being addressed in this commit.

Additionally, the tests have been made more comprehensive and varied by:

  • Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR).
  • Testing against false positives for attribute class using the same name as the function.
  • Ensure function import use statements are not flagged. We're not interested in those.
  • Safeguarding that function calls using PHP 5.6+ argument unpacking are not flagged.
  • Safeguarding that the function is not flagged when used as a PHP 8.1+ first class callable.
  • Adding more variations to the pre-existing tests:
    • Non-lowercase function call(s).
    • Fully qualified function calls.
    • Use PHP 7.3+ trailing comma's in a few function calls.
    • Use both single quoted as well as double quoted text strings.
    • Use other non-plain-text tokens in the passed parameters.
    • Multi-line function call.
    • Comments in function call.
    • $subject parameter passed as array.

Security/StaticStrreplace: add tests with optional fourth parameter

... to ensure and safeguard the sniff does not get confused over that parameter.

Ref: https://www.php.net/manual/en/function.str-replace.php

Security/StaticStrreplace: bug fix - false negatives for PHP 5.4+ short array parameters

Fixed now. Includes tests.
Note: some of the "should not be flagged" tests already use short arrays since the tests were expanded.

Security/StaticStrreplace: use PHPCSUtils for array parsing

As things were, the sniff walked the tokens in an array and expected a comma to always belong to the array, i.e. [ 'text', 'text' ].
This falls foul as soon as the code being walked gets slightly more complex, like using nested arrays or dynamic values in the array: [ 'text', [ $a, $b ], fnc( 'text', 'next' ) ].

Now, at this time, this is not strictly problematic for this sniff as it will bow out for any token which is not a T_CONSTANT_ENCAPSED_STRING, so would bow out for nested arrays and dynamic values.

Having said this, using the PHPCSUtils PassedParameters::getParameters() for parsing an array to it's individual items should still make the code more stable and also benefits from the PHPCSUtils build-in caching.

Security/StaticStrreplace: use pre-parsed function call argument information / support PHP 8.0+ named arguments

As things were, the sniff walked the tokens in the function call itself, but what with the switch to extending the AbstractFunctionParameterSniff class, we already have the start and end pointers of each passed parameter available, so let's start using that information instead of doing the token walking within the sniff.

By using the pre-parsed information, the sniff automatically will start supporting the use of PHP 8.0+ named arguments in function calls.

Includes tests.

Security/StaticStrreplace: handle more "static" tokens

This commit primarily addresses that the sniff did not take PHP 5.3 nowdocs into account as "static text" tokens.

However, heredocs can also be "static text" if there is no interpolation and there are some other variants of valid code which would still make the code effectively "static text".

Fixed now by checking the code more thoroughly.

The actual checking has been moved to a new is_parameter_static_text() method as if the passed parameter is an array, the same checks would need to be done again for each array item. By having the check in a separate method, the method can recursively call itself for array items.

Includes tests.

Closes #544

jrfnl added 6 commits July 18, 2025 17:34
As things were, the determination of whether or not a `T_STRING` is a call to the global PHP native `str_replace()` function was severely flawed.

By switching the sniff over to be based on the WordPressCS `AbstractFunctionParameterSniff` class, this flaw is mitigated.

Includes adding a slew of additional tests, some of which (line 8 - 13) are specific to the flaw being addressed in this commit.

Additionally, the tests have been made more comprehensive and varied by:
* Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR).
* Testing against false positives for attribute class using the same name as the function.
* Ensure function import `use` statements are not flagged. We're not interested in those.
* Safeguarding that function calls using PHP 5.6+ argument unpacking are not flagged.
* Safeguarding that the function is not flagged when used as a PHP 8.1+ first class callable.
* Adding more variations to the pre-existing tests:
    - Non-lowercase function call(s).
    - Fully qualified function calls.
    - Use PHP 7.3+ trailing comma's in a few function calls.
    - Use both single quoted as well as double quoted text strings.
    - Use other non-plain-text tokens in the passed parameters.
    - Multi-line function call.
    - Comments in function call.
    - `$subject` parameter passed as array.
... to ensure and safeguard the sniff does not get confused over that parameter.

Ref: https://www.php.net/manual/en/function.str-replace.php
…rt array parameters

Fixed now. Includes tests.
Note: some of the "should not be flagged" tests already use short arrays since the tests were expanded.
As things were, the sniff walked the tokens in an array and expected a comma to always belong to the array, i.e. `[ 'text', 'text' ]`.
This falls foul as soon as the code being walked gets slightly more complex, like using nested arrays or dynamic values in the array: `[ 'text', [ $a, $b ], fnc( 'text', 'next' ) ]`.

Now, at this time, this is not strictly problematic for this sniff as it will bow out for any token which is not a `T_CONSTANT_ENCAPSED_STRING`, so would bow out for nested arrays and dynamic values.

Having said this, using the PHPCSUtils `PassedParameters::getParameters()` for parsing an array to it's individual items should still make the code more stable and also benefits from the PHPCSUtils build-in caching.
…rmation / support PHP 8.0+ named arguments

As things were, the sniff walked the tokens in the function call itself, but what with the switch to extending the `AbstractFunctionParameterSniff` class, we already have the start and end pointers of each passed parameter available, so let's start using that information instead of doing the token walking within the sniff.

By using the pre-parsed information, the sniff automatically will start supporting the use of PHP 8.0+ named arguments in function calls.

Includes tests.
This commit primarily addresses that the sniff did not take PHP 5.3 nowdocs into account as "static text" tokens.

However, heredocs can also be "static text" if there is no interpolation and there are some other variants of valid code which would still make the code effectively "static text".

Fixed now by checking the code more thoroughly.

The actual checking has been moved to a new `is_parameter_static_text()` method as if the passed parameter is an array, the same checks would need to be done again for each array item. By having the check in a separate method, the method can recursively call itself for array items.

Includes tests.
@jrfnl jrfnl added this to the 3.1.0 milestone Jul 18, 2025
@jrfnl jrfnl requested a review from a team as a code owner July 18, 2025 15:40
@jrfnl jrfnl added Type: Bug Type: Enhancement Type: Maintenance PHPCSUtils The addition and utilisation of PHPCSUtils package labels Jul 18, 2025
@GaryJones GaryJones merged commit c949fb6 into develop Jul 18, 2025
42 checks passed
@GaryJones GaryJones deleted the feature/security-staticstrreplace-sniff-review branch July 18, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PHPCSUtils The addition and utilisation of PHPCSUtils package Type: Bug Type: Enhancement Type: Maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review the WordPressVIPMinimum.Security.StaticStrreplace sniff

2 participants